fix: clean up stale MCP sessions to bound memory growth#71
Conversation
📝 WalkthroughWalkthroughAdds an MCP session registry with idle cleanup and per-session close results. Server and CLI shutdown now await transport, application, and HTTP cleanup, with guarded signal handling and lifecycle tests for ordering, delays, and errors. ChangesMCP session and server shutdown lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SignalHandler
participant shutdownHttpServer
participant RunningServer
participant McpSessionRegistry
participant HTTPServer
SignalHandler->>shutdownHttpServer: request shutdown
shutdownHttpServer->>RunningServer: close application
RunningServer->>McpSessionRegistry: closeAll()
McpSessionRegistry->>McpSessionRegistry: close transports
shutdownHttpServer->>HTTPServer: close(callback)
HTTPServer-->>shutdownHttpServer: drain complete or error
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Thanks for the PR @Zhenyu98 — the idle-session cleanup is a meaningful RAM-usage improvement. The current session map can retain abandoned transports and their connected MCP server object graphs indefinitely, so bounding that lifecycle is a valuable fix for long-running DevSpace processes. The registry implementation itself looks sound, but the server-shutdown cleanup is currently ordered in a way that can prevent it from running. Both entrypoints call There is also a second race: Could we make const shutdown = async () => {
const httpClosed = new Promise<void>((resolve, reject) => {
httpServer.close((error) => {
if (error) reject(error);
else resolve();
});
});
await close();
await httpClosed;
};This lets closing the MCP transports terminate active SSE responses, which in turn allows |
|
Thanks for digging into this — that was a sharp catch. @Waishnav The original change was focused on bounding the lifetime of abandoned MCP sessions. Your shutdown analysis connected that fix to the server’s termination path and exposed two issues that had been missed: the circular wait between Both cases were reproduced, and the implementation was updated in Focused regression tests were added for shutdown ordering, delayed transport closure, HTTP drain completion, and HTTP close failures. The branch was also rebased onto the current |
880aeea to
ee372e7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/server-shutdown.ts (1)
9-17: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding a drain timeout for production resilience.
If a client holds an HTTP connection open indefinitely,
httpClosednever resolves andshutdownHttpServerhangs forever. A configurable timeout withhttpServer.closeAllConnections()(Node 18.2+) as a fallback would bound shutdown latency. This is a design trade-off, not a defect — flagging for awareness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server-shutdown.ts` around lines 9 - 17, Add a configurable drain timeout around the httpClosed wait in shutdownHttpServer, and when it expires invoke httpServer.closeAllConnections() before completing shutdown. Preserve normal graceful closure when connections drain within the timeout, and use the existing HTTP server lifecycle symbols rather than changing closeApplication.src/cli.ts (1)
232-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared shutdown handler to avoid duplication with
server.ts.The
shuttingDownguard,shutdown(),handleShutdown(), andprocess.oncepattern is identical toserver.ts:1845-1860. A shared helper likeinstallShutdownHandlers(httpServer, close)would prevent these copies from diverging.♻️ Suggested extraction
+ // src/shutdown-handler.ts + import type { ClosableHttpServer } from "./server-shutdown.js"; + import { shutdownHttpServer } from "./server-shutdown.js"; + + export function installShutdownHandlers( + httpServer: ClosableHttpServer, + close: () => Promise<void>, + ): void { + let shuttingDown = false; + const shutdown = async () => { + if (shuttingDown) return; + shuttingDown = true; + await shutdownHttpServer(httpServer, close); + process.exit(0); + }; + const handleShutdown = () => { + void shutdown().catch((error) => { + console.error("devspace shutdown failed", error); + process.exit(1); + }); + }; + process.once("SIGINT", handleShutdown); + process.once("SIGTERM", handleShutdown); + }Then in both
cli.tsandserver.ts:- let shuttingDown = false; - const shutdown = async () => { - if (shuttingDown) return; - shuttingDown = true; - await shutdownHttpServer(httpServer, close); - process.exit(0); - }; - const handleShutdown = () => { - void shutdown().catch((error) => { - console.error("devspace shutdown failed", error); - process.exit(1); - }); - }; - process.once("SIGINT", handleShutdown); - process.once("SIGTERM", handleShutdown); + installShutdownHandlers(httpServer, close);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.ts` around lines 232 - 246, Extract the duplicated shutdown flow into a shared installShutdownHandlers helper, including the shuttingDown guard, shutdown() cleanup, handleShutdown() error handling, and SIGINT/SIGTERM process.once registrations. Replace the inline implementations in both cli.ts and server.ts with calls to this helper, passing httpServer and close while preserving existing exit behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/cli.ts`:
- Around line 232-246: Extract the duplicated shutdown flow into a shared
installShutdownHandlers helper, including the shuttingDown guard, shutdown()
cleanup, handleShutdown() error handling, and SIGINT/SIGTERM process.once
registrations. Replace the inline implementations in both cli.ts and server.ts
with calls to this helper, passing httpServer and close while preserving
existing exit behavior.
In `@src/server-shutdown.ts`:
- Around line 9-17: Add a configurable drain timeout around the httpClosed wait
in shutdownHttpServer, and when it expires invoke
httpServer.closeAllConnections() before completing shutdown. Preserve normal
graceful closure when connections drain within the timeout, and use the existing
HTTP server lifecycle symbols rather than changing closeApplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 904cec6a-8d64-42f5-9ef2-e788d9aafece
📒 Files selected for processing (6)
package.jsonsrc/cli.tssrc/mcp-sessions.test.tssrc/server-shutdown.test.tssrc/server-shutdown.tssrc/server.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/server.ts
Adapt Waishnav#71 to the maintained fork and make the idle timeout configurable. Co-authored-by: Zhenyu Wu <[email protected]>
Summary
McpSessionRegistrythat tracks the last activity time for each Streamable HTTP transportMotivation
createServer()currently keeps every initialized MCP transport in a process-lifetimeMapand removes it only whentransport.onclosefires. Some MCP clients reconnect without explicitly closing the previous transport, so abandoned transports and their connected MCP server/tool objects can remain strongly referenced indefinitely.In one long-running deployment, the logs contained 96
mcp_session_createdevents and zeromcp_session_closedevents. The DevSpace process grew from about 218 MB of private memory immediately after a restart to about 2.55 GB after roughly two weeks, then returned to the baseline after restarting. This observation does not prove that every retained byte came from MCP transports, but it demonstrates the unbounded-retention risk and motivated this memory-usage fix.Behavior
transport.close()is awaited, so references are released even when closing failsUnknown MCP sessionresponse and can initialize a new sessiontransport.onclosehandling remains supported and no duplicate close log is emittedVerification
npm testnpm run typechecknpm run buildgit diff upstream/main...HEAD --checkThe change is intentionally limited to MCP transport lifecycle management. Workspace-cache eviction can be evaluated separately.
Summary to CodeRabbit
New Features
Bug Fixes
Tests